-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a set for file names for clash checking #2422
Conversation
@@ -455,12 +457,10 @@ def writeGlyph(self, glyphName, glyphObject=None, drawPointsFunc=None, formatVer | |||
fileName = self.contents.get(glyphName) | |||
if fileName is None: | |||
if self._existingFileNames is None: | |||
self._existingFileNames = {} | |||
for fileName in self.contents.values(): | |||
self._existingFileNames[fileName] = fileName.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why this self._existingFileNames
was a dict
to begin with, if we were only using the .values().
I hope nobody else ever accessed that (well, it is prefixed with underscore so technically it's private)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Just and Nikolaus!
e2b788c
to
cd32e1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
Closes #2421.
Thanks @justvanrossum, right on the money :) In the mentioned script, runtime spent in
glyphNameToFileName
dropped from 20% to 0.15%, that's a 2 minutes reduction on my machine :)I smuggled a type annotation in and directed Python to postpone evaluation of annotations, to avoid the parsing overhead and also because the
set[str] | None
syntax is Python 3.9+.